Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clarify kill and delete operation for shared pid namespace container #1234

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lifubang
Copy link
Member

@lifubang lifubang commented Oct 6, 2023

There was no description for delete a container which doesn't have it's own private pid namespace, so it may cause some break changes when we do code refactor in this area.
Maybe we should add some descriptions for this.


In fact, container-runtime kill [-a,--all] container_id signal] and container-runtime delete [-f,--force] container_id has been used by upstream projects for many years, but it isn't defined in runtime-spec. It may cause some break changes when we do code refactor in this area.
So, maybe we should define it clearly in runtime-spec.

background: opencontainers/runc#4049

runtime.md Outdated Show resolved Hide resolved
@lifubang
Copy link
Member Author

lifubang commented Oct 8, 2023

@opencontainers/runtime-spec-maintainers PTAL, we need this PR to make a decision how to fix a bug in runc(opencontainers/runc#4047).
Looking forward your feedback, thanks.

runtime.md Outdated Show resolved Hide resolved
runtime.md Outdated
@@ -129,18 +129,19 @@ This operation MUST run the user-specified program as specified by [`process`](c
This operation MUST generate an error if `process` was not set.

### <a name="runtimeKill" />Kill
`kill <container-id> <signal>`
`kill <container-id> [-a,--all] <signal>`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I changed this PR to clarify kill and delete operation for shared pid namespace container, let's discuss -a in #1190 .

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: these operations are not specifying any command-line APIs, and the parameters are inputs for general operations.

@lifubang lifubang changed the title clarify kill and delete operation clarify kill and delete operation for shared pid namespace container Oct 10, 2023
@lifubang
Copy link
Member Author

Note: these operations are not specifying any command-line APIs, and the parameters are inputs for general operations.

I wonder we should change this rule or not?
Before we change this rule, I changed my PR to clarify kill and delete operation for shared pid namespace container, if we don't think this type container should be described in runtime-spec, feel free to close this PR.

@fuweid
Copy link
Member

fuweid commented Oct 11, 2023

@lifubang the operation can be function or command-line. -a is kind of command-line api. I think the maintainer can accept <all> here.

However, the runc kill -a has been used for many years. <all> can be -a in the command-line implementation, is it correct? @AkihiroSuda

@AkihiroSuda
Copy link
Member

However, the runc kill -a has been used for many years. can be -a in the command-line implementation, is it correct? @AkihiroSuda

Yes, but the command line implementation is (currently) out of the scope of the OCI Runtime Spec.

@lifubang
Copy link
Member Author

Yes, but the command line implementation is (currently) out of the scope of the OCI Runtime Spec.

How about add runtime-cmd-api.md to define all the basic command apis?

@AkihiroSuda
Copy link
Member

Yes, but the command line implementation is (currently) out of the scope of the OCI Runtime Spec.

How about add runtime-cmd-api.md to define all the basic command apis?

Adding the CLI spec to the Runtime Spec was once rejected in 2017 (#513 (comment)) and accepted in the runtime-tools repo: https://github.com/opencontainers/runtime-tools/blob/master/docs/command-line-interface.md

I guess we can revisit this, but it is beyond the scope of this PR. (Feel free to open an issue/PR)

@fuweid
Copy link
Member

fuweid commented Oct 11, 2023

Yes, but the command line implementation is (currently) out of the scope of the OCI Runtime Spec.

How about add runtime-cmd-api.md to define all the basic command apis?

Adding the CLI spec to the Runtime Spec was once rejected in 2017 (#513 (comment)) and accepted in the runtime-tools repo: https://github.com/opencontainers/runtime-tools/blob/master/docs/command-line-interface.md

I guess we can revisit this, but it is beyond the scope of this PR. (Feel free to open an issue/PR)

Thanks. We should discuss it in other pr (I also received the runc maintainer's comment about introducing new flag opencontainers/runc#4045)

runtime.md Outdated

This operation MUST send the specified signal to the container's init process.

Specially, if the signal is `SIGKILL` and the container does not use its own private PID namespace, this operation MUST send the `SIGKILL` signal to all the processes in the container, even if the container's state is `stopped`. If there is no process left in this type container, the operation MUST [generate an error](#errors).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. s/does not use/does not have/
  2. Traditionally, runc kill -a does not result in an error even if there are no processes left. I'm not sure if we want to change it here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2. Traditionally, runc kill -a does not result in an error even if there are no processes left. I'm not sure if we want to change it here.

Yes, I think we should not result in an error here, I'll remove these words later.

runtime.md Outdated
Deleting a container MUST delete the resources that were created during the `create` step.

Specially, when deleting a container, which does not use its own private PID namespace, the operation should ensure kill all processes in this type container, and ensure no process left in it.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/does not use/does not have/

s/should ensure kill all processes/SHOULD kill all processes/

s/and ensure no process left in it./and return an error if those processes can not be killed./

@lifubang
Copy link
Member Author

lifubang commented Dec 6, 2023

@opencontainers/runtime-spec-maintainers PTAL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants